From eb825f79cb6e0939871180692808f241fe5ddc93 Mon Sep 17 00:00:00 2001 From: "cl349@firebug.cl.cam.ac.uk" Date: Tue, 26 Jul 2005 15:26:32 +0000 Subject: [PATCH] Remove ill-conceived concept of watches blocking reply on connection which did write/mkdir/rm/setperm etc. This causes deadlocks in real life, and I can't see a sane way of avoiding them: it is reasonable for someone to ignore watch notifications while doing other actions, and that means that we can do other writes. These writes can block pending other watchers; if one of these is the process blocked awaiting our ack, we deadlock. Signed-off-by: Rusty Russel Signed-off-by: Christian Limpach --- tools/xenstore/xenstored_core.c | 29 ++++---------------------- tools/xenstore/xenstored_core.h | 8 ------- tools/xenstore/xenstored_transaction.c | 9 ++------ tools/xenstore/xenstored_watch.c | 21 +++++-------------- tools/xenstore/xenstored_watch.h | 3 +-- 5 files changed, 12 insertions(+), 58 deletions(-) diff --git a/tools/xenstore/xenstored_core.c b/tools/xenstore/xenstored_core.c index 3bf63314b7..e257853cb2 100644 --- a/tools/xenstore/xenstored_core.c +++ b/tools/xenstore/xenstored_core.c @@ -976,10 +976,7 @@ static void do_write(struct connection *conn, struct buffered_data *in) } add_change_node(conn->transaction, node, false); - if (fire_watches(conn, node, false)) { - conn->watch_ack = XS_WRITE; - return; - } + fire_watches(conn, node, false); send_ack(conn, XS_WRITE); } @@ -1005,10 +1002,7 @@ static void do_mkdir(struct connection *conn, const char *node) } add_change_node(conn->transaction, node, false); - if (fire_watches(conn, node, false)) { - conn->watch_ack = XS_MKDIR; - return; - } + fire_watches(conn, node, false); send_ack(conn, XS_MKDIR); } @@ -1046,10 +1040,7 @@ static void do_rm(struct connection *conn, const char *node) } add_change_node(conn->transaction, node, true); - if (fire_watches(conn, node, true)) { - conn->watch_ack = XS_RM; - return; - } + fire_watches(conn, node, true); send_ack(conn, XS_RM); } @@ -1121,10 +1112,7 @@ static void do_set_perms(struct connection *conn, struct buffered_data *in) } add_change_node(conn->transaction, node, false); - if (fire_watches(conn, node, false)) { - conn->watch_ack = XS_SET_PERMS; - return; - } + fire_watches(conn, node, false); send_ack(conn, XS_SET_PERMS); } @@ -1359,12 +1347,6 @@ static void unblock_connections(void) consider_message(i); } break; - case WATCHED: - if (i->watches_unacked == 0) { - i->state = OK; - send_ack(i, i->watch_ack); - } - break; case OK: break; } @@ -1389,8 +1371,6 @@ struct connection *new_connection(connwritefn_t *write, connreadfn_t *read) new->state = OK; new->blocked_by = NULL; - new->watch_ack = XS_ERROR; - new->watches_unacked = 0; new->out = new->waiting_reply = NULL; new->fd = -1; new->id = 0; @@ -1471,7 +1451,6 @@ void dump_connection(void) printf(" state = %s\n", i->state == OK ? "OK" : i->state == BLOCKED ? "BLOCKED" - : i->state == WATCHED ? "WATCHED" : "INVALID"); if (i->id) printf(" id = %i\n", i->id); diff --git a/tools/xenstore/xenstored_core.h b/tools/xenstore/xenstored_core.h index 9643d36b95..22662a2eb7 100644 --- a/tools/xenstore/xenstored_core.h +++ b/tools/xenstore/xenstored_core.h @@ -51,8 +51,6 @@ enum state { /* Blocked by transaction. */ BLOCKED, - /* Waiting for watchers to ack event we caused */ - WATCHED, /* Completed */ OK, }; @@ -73,12 +71,6 @@ struct connection /* Node we are waiting for (if state == BLOCKED) */ char *blocked_by; - /* Are we waiting for watches to be acked from an event we caused? */ - unsigned int watches_unacked; - - /* Type of ack to send once watches fired. */ - enum xsd_sockmsg_type watch_ack; - /* Is this a read-only connection? */ bool can_write; diff --git a/tools/xenstore/xenstored_transaction.c b/tools/xenstore/xenstored_transaction.c index afaef1bef2..eef4d29038 100644 --- a/tools/xenstore/xenstored_transaction.c +++ b/tools/xenstore/xenstored_transaction.c @@ -307,7 +307,6 @@ void do_transaction_end(struct connection *conn, const char *arg) { struct changed_node *i; struct transaction *trans; - bool fired = false; if (!arg || (!streq(arg, "T") && !streq(arg, "F"))) { send_error(conn, EINVAL); @@ -337,12 +336,8 @@ void do_transaction_end(struct connection *conn, const char *arg) /* Fire off the watches for everything that changed. */ list_for_each_entry(i, &trans->changes, list) - fired |= fire_watches(conn, i->node, i->recurse); + fire_watches(conn, i->node, i->recurse); } - - if (fired) - conn->watch_ack = XS_TRANSACTION_END; - else - send_ack(conn, XS_TRANSACTION_END); + send_ack(conn, XS_TRANSACTION_END); } diff --git a/tools/xenstore/xenstored_watch.c b/tools/xenstore/xenstored_watch.c index a49dcbc954..1c42b72ced 100644 --- a/tools/xenstore/xenstored_watch.c +++ b/tools/xenstore/xenstored_watch.c @@ -41,9 +41,6 @@ struct watch_event /* Data to send (node\0token\0). */ unsigned int len; char *data; - - /* Connection which caused watch event (which we are blocking) */ - struct connection *cause; }; struct watch @@ -95,14 +92,10 @@ static int destroy_watch_event(void *_event) struct watch_event *event = _event; trace_destroy(event, "watch_event"); - assert(event->cause->watches_unacked != 0); - /* If it hits zero, will unblock in unblock_connections. */ - event->cause->watches_unacked--; return 0; } -static void add_event(struct connection *cause, struct watch *watch, - const char *node) +static void add_event(struct watch *watch, const char *node) { struct watch_event *event; @@ -117,22 +110,20 @@ static void add_event(struct connection *cause, struct watch *watch, event->data = talloc_array(event, char, event->len); strcpy(event->data, node); strcpy(event->data + strlen(node) + 1, watch->token); - event->cause = cause; - cause->watches_unacked++; talloc_set_destructor(event, destroy_watch_event); list_add_tail(&event->list, &watch->events); trace_create(event, "watch_event"); } /* FIXME: we fail to fire on out of memory. Should drop connections. */ -bool fire_watches(struct connection *conn, const char *node, bool recurse) +void fire_watches(struct connection *conn, const char *node, bool recurse) { struct connection *i; struct watch *watch; /* During transactions, don't fire watches. */ if (conn->transaction) - return false; + return; /* Create an event for each watch. Don't send to self. */ list_for_each_entry(i, &connections, list) { @@ -141,18 +132,16 @@ bool fire_watches(struct connection *conn, const char *node, bool recurse) list_for_each_entry(watch, &i->watches, list) { if (is_child(node, watch->node)) - add_event(conn, watch, node); + add_event(watch, node); else if (recurse && is_child(watch->node, node)) - add_event(conn, watch, watch->node); + add_event(watch, watch->node); else continue; - conn->state = WATCHED; /* If connection not doing anything, queue this. */ if (!i->out) queue_next_event(i); } } - return conn->state == WATCHED; } static int destroy_watch(void *_watch) diff --git a/tools/xenstore/xenstored_watch.h b/tools/xenstore/xenstored_watch.h index d1fac70502..30d7e9e172 100644 --- a/tools/xenstore/xenstored_watch.h +++ b/tools/xenstore/xenstored_watch.h @@ -33,9 +33,8 @@ bool is_watch_event(struct connection *conn, struct buffered_data *out); void queue_next_event(struct connection *conn); /* Fire all watches: recurse means all the children are effected (ie. rm). - * Returns true if there were any, meaning connection has to wait. */ -bool fire_watches(struct connection *conn, const char *node, bool recurse); +void fire_watches(struct connection *conn, const char *node, bool recurse); /* Find shortest timeout: if any, reduce tv (may already be set). */ void shortest_watch_ack_timeout(struct timeval *tv); -- 2.30.2